Simplify SPDX compound expressions #9360
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9360 +/- ##
=========================================
Coverage 67.67% 67.67%
Complexity 1223 1223
=========================================
Files 244 244
Lines 8626 8626
Branches 911 911
=========================================
Hits 5838 5838
Misses 2413 2413
Partials 375 375
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
sschuberth
force-pushed
the
spdx-simplify-compound
branch
from
8daf030
to
42429f3
Compare
Improve the overview before adding more tests. Signed-off-by: Sebastian Schuberth <[email protected]>
Do not even create a copy if operands are equal. Resolves #8714. Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth
force-pushed
the
spdx-simplify-compound
branch
from
041eb86
to
1201aba
Compare
|
|
||
| /** | ||
| * Concatenate [this][SpdxExpression] and [other] using [SpdxOperator.OR]. | ||
| */ | ||
| infix fun or(other: SpdxExpression) = | ||
| takeIf { this == other } ?: SpdxCompoundExpression(SpdxOperator.OR, listOf(this, other)) | ||
| takeIf { this == other } ?: SpdxCompoundExpression(SpdxOperator.OR, setOf(this, other)) |
There was a problem hiding this comment.
With the takeIf expression the setOf here and above should not have any effect except for unnecessarily repeating the comparison.
There was a problem hiding this comment.
I'll simply drop this commit to address this and the below issue.
| @@ -224,14 +224,14 @@ class SpdxCompoundExpression( | |||
| * Create a compound expression with the provided [operator] and the [left] and [right] child expressions. | |||
| */ | |||
| constructor(left: SpdxExpression, operator: SpdxOperator, right: SpdxExpression) : | |||
| this(operator, listOf(left, right)) | |||
| this(operator, setOf(left, right)) | |||
There was a problem hiding this comment.
This could run into an exception as SpdxCompoundExpression requires that children.size > 1. So basically this change now forbids to make compound expressions with equals operands.
There was a problem hiding this comment.
I'll simply drop this commit to address this and the above issue.
| @@ -34,4 +35,37 @@ class SpdxCompoundExpressionTest : WordSpec({ | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| "Simplifying a compound expression" should { | |||
| "nested children of the same operator" { | |||
There was a problem hiding this comment.
Word missing in the beginning.
| } | ||
| } | ||
|
|
||
| return SpdxCompoundExpression(operator, flattenedChildren) |
There was a problem hiding this comment.
Maybe this function should be called by normalize as well. Also, this will throw an exception if flattenedChildren.size < 2.
There was a problem hiding this comment.
Maybe this function should be called by
normalizeas well.
I have a slight tendency towards keeping normalizing and simplifying separate use-cases.
However, having the respective File functions in mind, we should have probably called the current normalize rather canonize, leaving room for calling this function normalize instead of simplify.
Also, this will throw an exception if flattenedChildren.size < 2.
Good find. Fixed, and added a test for that case.
| @@ -204,6 +204,7 @@ internal fun Package.toSpdxPackage( | |||
| SpdxConstants.NONE | |||
| } else { | |||
| packageLicenseExpressions.reduce(SpdxExpression::and) | |||
| .simplify() | |||
There was a problem hiding this comment.
This will now throw an exception if the input contains only two identical licenses.
There was a problem hiding this comment.
Fixed, see my comment above.
Signed-off-by: Sebastian Schuberth <[email protected]>
Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth
force-pushed
the
spdx-simplify-compound
branch
from
1201aba
to
c9048f3
Compare
Please have a look at the individual commit messages for the details.